fix(3125): merge parent build meta order by end time #481
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
VonnyJap
reviewed
Nov 18, 2024
launch.go
Outdated
| parentBuild, err := api.BuildFromID(parentBuildID) | ||
| if err != nil { | ||
| return resultMeta, fmt.Errorf("Fetching Parent Build ID %d: %v", parentBuildID, err) | ||
| var join = len(parentBuildIds) > 1 |
There was a problem hiding this comment.
Suggested change
| var join = len(parentBuildIds) > 1 | |
| var isJoin = len(parentBuildIds) > 1 |
launch.go
Outdated
Comment on lines
332
to
358
| // Write to "sd@123:component.json", where sd@123:component is the triggering job | ||
| externalMetaFile := "sd@" + strconv.Itoa(parentJob.PipelineID) + ":" + parentJob.Name + ".json" | ||
| writeMetafile(metaSpace, externalMetaFile, metaLog, parentBuild.Meta) | ||
| if join { | ||
| marshallValue, err := json.Marshal(parentBuild.Meta) | ||
| if err != nil { | ||
| return resultMeta, fmt.Errorf("Cloning meta of Parent Build ID %d: %v", parentBuild.ID, err) | ||
| } | ||
| var externalParentBuildMeta map[string]interface{} | ||
| json.Unmarshal(marshallValue, &externalParentBuildMeta) | ||
|
|
||
| // Always exclude parameters from external meta | ||
| delete(externalParentBuildMeta, "parameters") | ||
|
|
||
| // delete local version of external meta | ||
| pIDString := strconv.Itoa(parentJob.PipelineID) | ||
| pjn := parentJob.Name | ||
| if sdMeta, ok := resultMeta["sd"]; ok { | ||
| if externalPipelineMeta, ok := sdMeta.(map[string]interface{})[pIDString]; ok { | ||
| if _, ok := externalPipelineMeta.(map[string]interface{})[pjn]; ok { | ||
| delete(externalPipelineMeta.(map[string]interface{}), pjn) | ||
| resultMeta = deepMergeJSON(resultMeta, externalParentBuildMeta) | ||
| } | ||
|
|
||
| // delete local version of external meta | ||
| pIDString := strconv.Itoa(parentJob.PipelineID) | ||
| pjn := parentJob.Name | ||
| if sdMeta, ok := resultMeta["sd"]; ok { | ||
| if externalPipelineMeta, ok := sdMeta.(map[string]interface{})[pIDString]; ok { | ||
| if _, ok := externalPipelineMeta.(map[string]interface{})[pjn]; ok { | ||
| delete(externalPipelineMeta.(map[string]interface{}), pjn) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
please separate this logic to a different function, for example:
func handleExternalPipelineMeta(api screwdriver.API, parentBuild screwdriver.Build, parentJob screwdriver.Job, resultMeta map[string]interface{}, metaSpace, metaLog string, isJoin bool) (map[string]interface{}, error) {
externalMetaFile := ExternalMetaFilePrefix + strconv.Itoa(parentJob.PipelineID) + ":" + parentJob.Name + ExternalMetaFileSuffix
writeMetafile(metaSpace, externalMetaFile, metaLog, parentBuild.Meta)
if isJoin {
marshallValue, err := json.Marshal(parentBuild.Meta)
if err != nil {
return resultMeta, fmt.Errorf("Cloning meta of Parent Build ID %d: %v", parentBuild.ID, err)
}
var externalParentBuildMeta map[string]interface{}
json.Unmarshal(marshallValue, &externalParentBuildMeta)
delete(externalParentBuildMeta, "parameters")
resultMeta = deepMergeJSON(resultMeta, externalParentBuildMeta)
}
pIDString := strconv.Itoa(parentJob.PipelineID)
pjn := parentJob.Name
if sdMeta, ok := resultMeta["sd"]; ok {
if externalPipelineMeta, ok := sdMeta.(map[string]interface{})[pIDString]; ok {
if _, ok := externalPipelineMeta.(map[string]interface{})[pjn]; ok {
delete(externalPipelineMeta.(map[string]interface{}), pjn)
}
}
}
return resultMeta, nil
}
this will help the code to be more readable.
There was a problem hiding this comment.
Thank you for the suggestion. I fixed it 🙇
yk634
reviewed
Nov 20, 2024
launch.go
Outdated
| resultMeta, err = handleExternalPipelineMeta(parentBuild, parentJob, resultMeta, metaSpace, metaLog, isJoin) | ||
|
|
||
| if err != nil { | ||
| return resultMeta, fmt.Errorf("Mergeing meta of External Parent Build ID %d: %v", parentBuild.ID, err) |
There was a problem hiding this comment.
[nits] I think it's a spelling mistake.
Suggested change
| return resultMeta, fmt.Errorf("Mergeing meta of External Parent Build ID %d: %v", parentBuild.ID, err) | |
| return resultMeta, fmt.Errorf("Merging meta of External Parent Build ID %d: %v", parentBuild.ID, err) |
yk634
reviewed
Nov 20, 2024
launch.go
Outdated
| // SetExternalMeta checks if parent build is external and sets meta in external file accordingly | ||
| func SetExternalMeta(api screwdriver.API, pipelineID, parentBuildID int, mergedMeta map[string]interface{}, metaSpace, metaLog string, join bool) (map[string]interface{}, error) { | ||
| // setParentBuildsMeta checks if parent build is external and sets meta in external file accordingly | ||
| func setParentBuildsMeta(api screwdriver.API, pipelineID int, parentBuildIds []int, mergedMeta map[string]interface{}, metaSpace, metaLog string) (map[string]interface{}, error) { |
There was a problem hiding this comment.
[nits] In other places it is “parentBuildIDs”, so it might be better to be consistent.
Suggested change
| func setParentBuildsMeta(api screwdriver.API, pipelineID int, parentBuildIds []int, mergedMeta map[string]interface{}, metaSpace, metaLog string) (map[string]interface{}, error) { | |
| func setParentBuildsMeta(api screwdriver.API, pipelineID int, parentBuildIDs []int, mergedMeta map[string]interface{}, metaSpace, metaLog string) (map[string]interface{}, error) { |
yk634
approved these changes
Nov 21, 2024
|
@y-oksaku - please rebase your branch and I will approve after that. |
VonnyJap
approved these changes
Nov 23, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Context
Currently, if the parent builds have same key metadata, the value of the build completed earlier is used.
(Not value of the build completed later)
This behavior is a little strange.
Objective
Sort merging parent builds metadata order by their finished time.
References
screwdriver-cd/screwdriver#3125 (comment)
License
I confirm that this contribution is made under the terms of the license found in the root directory of this repository's source tree and that I have the authority necessary to make this contribution on behalf of its copyright owner.